Fix match exhaustiveness and generic preservation for ::class comparisons#5305
Fix match exhaustiveness and generic preservation for ::class comparisons#5305mhert wants to merge 2 commits intophpstan:2.1.xfrom
Conversation
|
You've opened the pull request against the latest branch 2.2.x. PHPStan 2.2 is not going to be released for months. If your code is relevant on 2.1.x and you want it to be released sooner, please rebase your pull request and change its target to 2.1.x. |
e046593 to
3b9ff34
Compare
3b9ff34 to
708c2c2
Compare
|
This pull request has been marked as ready for review. |
708c2c2 to
23b1556
Compare
…ss for sealed classes Closes phpstan/phpstan#12241 When using match($foo::class) on a @phpstan-sealed class hierarchy, PHPStan reported "Match expression does not handle remaining value" even when all allowed subtypes were covered. This happened because GenericClassStringType::tryRemove() did not consult the sealed hierarchy's allowed subtypes when removing a class-string constant. Extended tryRemove() to progressively subtract allowed subtypes from the class-string type. Each match arm removes one subtype until all are exhausted and the type becomes never, making the match exhaustive.
…lass comparison When narrowing a union type like Cat<string>|Dog<string> via $a::class === Cat::class, the generic type parameters were discarded — the narrowed type became plain Cat instead of Cat<string>. This caused method return types to be inferred as mixed instead of the concrete generic parameter. Added narrowTypePreservingGenerics() to TypeSpecifier which preserves generic information through two strategies: intersecting with the current union type (which lets TypeCombinator distribute and pick the matching generic member), and inferring template parameters through the class hierarchy for single generic parent types.
c3bfa1d to
ab7cbdd
Compare
|
this looks like a very big AI generated PR. @mhert did review it yourself and double checked whether it makes sense to you? |
I'm not an expert in phpstan's internals, but I did my best, to try to understand what's happening, and I think the AI did a good job. I also fixed one possible performance bottleneck (unnecessary foreach in the generated code) by myself. I also did let the AI do some reviews and let the upgraded phpstan run against some repositories I work with. I didn't notice any problems in correctness or performance. So I'm sure that this PR is okay. Thanks for looking in 😊 |
|
I can tell it’s wrong just be looking at the diff size, sorry 😊 |
As there are two fixes, should I create two pull requests, to make it easier to review (the commits are separated by topic already)? And if the diff size is too big, should I add less tests? It's "only" +130 without tests. And if you think the implementation is wrong, could you please give me a hint of what to do instead, to resolve phpstan/phpstan#12241 and the narrowing of a generic union with ::class. I would love to get this implemented and I will do my very best, to make it possible! Edit: @ondrejmirtes Looks like your comment made Claude to overthink the implmentation. Let me update my PR. Thanks for the hint! |
Summary
Two related fixes for
::classcomparison type narrowing in match expressions and if-else conditions.match($foo::class)on a@phpstan-sealedhierarchy falsely reported "Match expression does not handle remaining value" even when all allowed subtypes were covered.Cat<string>|Dog<string>via$a::class === Cat::classdiscarded generic type parameters — the type became plainCatinstead ofCat<string>.Closes phpstan/phpstan#12241
Root Cause
Sealed exhaustiveness:
GenericClassStringType::tryRemove()only handled final classes — it had no awareness of@phpstan-sealedhierarchies. Removing a class-string constant for a non-final sealed subtype was a no-op, so the type was never fully exhausted tonever.Generic preservation: The
$a::class === 'Foo'handler inTypeSpecifier::resolveNormalizedIdentical()constructed a plainObjectTypewithout consulting the current variable type for generic information. The generic parameters from the original union were lost.Fix
Sealed exhaustiveness: Extended
GenericClassStringType::tryRemove()to consultClassReflection::getAllowedSubTypes(). Each match arm progressively subtracts one allowed subtype viaObjectTypesubtraction. When all subtypes are removed, the type becomesnever, making the match exhaustive. Special-cases concrete (non-abstract) sealed parents to avoid self-referential subtraction.Generic preservation: Added
TypeSpecifier::narrowTypePreservingGenerics()which preserves generics through two strategies:TypeCombinator::intersect()distributes over members and picks the matching generic memberinferTemplateTypes()Test Plan
MatchExpressionRuleTest::testSealedClassStringMatch— exhaustive matches produce no error, partial matches report remaining valuensrt/sealed-class-string-match.php— progressive class-string narrowing, concrete parents, sealed interfaces, falsy branch (!==), match expressionsnsrt/class-string-match-preserves-generics.php— generic preservation for unions, single parents, final classes, mirror syntax, match arms with method callsCo-Authored-By: Claude Code